Add custom sg as an input to RDS#68
Add custom sg as an input to RDS#68lukaspour wants to merge 5 commits intoTeliaSoneraNorge:masterfrom
Conversation
antonbabenko
left a comment
antonbabenko
left a comment
There was a problem hiding this comment.
Mostly correct, few naming corrections.
| ingress_rules = ["${var.ingress_rule}"] | ||
| } | ||
|
|
||
| module "custom_security_group" { |
There was a problem hiding this comment.
Replace ecs in all arguments to this module with something like custom.
Word "custom" is more suitable for this module.
There was a problem hiding this comment.
All ecs references got removed
rds-instance-full/main.tf
Outdated
| ingress_with_source_security_group_id = [ | ||
| { | ||
| rule = "${var.ingress_rule}" | ||
| source_security_group_id = "${var.rds_sg}" |
There was a problem hiding this comment.
rds_sg => custom_sg_id
There was a problem hiding this comment.
Naming this is the biggest problem of IT, thanks!
| } | ||
|
|
||
| locals { | ||
| security_group_id = "${coalesce(join("", module.custom_security_group.*.security_group_id), module.rds_security_group.this_security_group_id)}" |
There was a problem hiding this comment.
If I remember correctly, join("", module.custom_security_group.*.security_group_id) can be replaced with module.custom_security_group.security_group_id.
There was a problem hiding this comment.
No, it didn't work when I was working on it as addition for the RDS module.
There was a problem hiding this comment.
@antonbabenko I will test this in RDS Neo module anyway, so I can do new PR removing this if it wouldn't be necessary
rds-instance-full/variables.tf
Outdated
| } | ||
|
|
||
| variable "rds_sg" { | ||
| description = "RDS security group" |
There was a problem hiding this comment.
"Custom security group id which should be allowed to have access to this RDS instance"
rds-instance-full/main.tf
Outdated
|
|
||
| name = "${local.identifier}-rds-ecs" | ||
| name = "${local.identifier}-rds-custom" | ||
| description = "Security group with RDS ports open for ECS" |
There was a problem hiding this comment.
"ECS" => "Security group with RDS ports open for a custom security group"
There was a problem hiding this comment.
I missed that one, thanks!
|
@antonbabenko what do you think, is it better now? I guess it is strongly related to this issue #69 if I am not able to use variables like |
|
Might be a silly question but why not just add a new rule to the existing security group exposed through the output |
|
To be honest, I am not sure anymore, this issue is going on for month now. We went through the problems around it in this issue #41 and made a call with Anton about it. I guess we have assumed that it would be easier to use separate SG for it because of some errors I found (I can try it again). But if it would work, it would save us few lines. |
3 participants
I guess this is what you meant in comments of this closed PR.